Switch from CliWrap to Shellfish #1046
Conversation
| @@ -119,7 +118,7 @@ public async Task VersionCommandTextFormat(TentacleConfigurationTestCase tc) | |||
|
|
|||
| var expectedVersion = GetVersionInfo(tc); | |||
|
|
|||
| stdout.Should().Be(expectedVersion.ProductVersion, "The version command should print the informational version as text"); | |||
| stdout.TrimEnd().Should().Be(expectedVersion.ProductVersion, "The version command should print the informational version as text"); | |||
There was a problem hiding this comment.
CliWrap deals with text output in a slightly different way to Shellfish.
If the app emits text without a trailing newline, CliWrap wouldn't give you a trailing newline either, whereas with Shellfish we get a trailing newline.
This isn't an inherent flaw in Shellfish, but rather just a side effect WithStdOutTarget(stringBuilder) output mechanism. The stringbuilder-writer puts a newline on the end of everything to keep things simple. We could do WithStdOutTarget(someCustomTarget) and get the output in a more direct way, but that would be harder and more complex than simply trimming the newlines in a few places
There was a problem hiding this comment.
For the test I don't think it matters.
|
No doubt the build has to actually work reliably before merging ;p |
Yeah I don't know what happened there, the tests just stalled forever on Win10 LTSC with no kind of log or anything, but they worked fine all the other OSes. Not a single test ran... I don't think it had anything to do with the Shellfish change, I'm re-running |
|
I guess I should comment about the context of this change. We used to use silent process runner but it being non async meant we ran out of threads which caused hangs, switching to CLIWrap fixed that. Now that silent process runner is async, the tests here seems like a good place to use that to verify it works well before we use it within Octopus or other related components. I think this PR makes sense, and I very much agree with the dogfooding approach. |
Background
Tentacle uses CliWrap for a handful of tests because it supports async execution.
our own Shellfish library has been recently enhanced to support this too.
Results
In the interests of dogfooding our own code, let's use Shellfish instead of CliWrap in the tentacle tests.
How to review this PR
Quality ✔️
Just a few changes to tests. If the tests pass, we should be good
Pre-requisites